Feature/preallocate_measurements #83
Conversation
There was a problem hiding this comment.
Pull request overview
This PR optimizes the get_measurements method in HerculesInterface for improved performance. The optimization focuses on eliminating repeated string formatting operations and reducing dictionary lookup overhead, achieving an approximately 2.5x speedup according to profiling results.
Key changes:
- Pre-computes LMP day-ahead keys once in
__init__instead of formatting them on every call - Caches
h_dict["external_signals"]in a local variable to avoid repeated dictionary lookups - Refactors the forecast loop to use
.items()instead of.keys()for more efficient iteration
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Full case run is 10% faster, so less impact when all-in, but probably still worth it |
There was a problem hiding this comment.
I'm not convinced that all of the changes are needed here, in particular, the separate declaration of the temporary external_signals variable. Even the main change, setting a new attribute _lmp_da_keys, is a bit borderline because it seems to trade readability (["lmp_da_{:02d}".format(h)] for h in range(24)) for something slightly more obscure (k in self._lmp_da_keys). At 10% improvement in end-to-end performance (thanks for reporting this, @paulf81 !), I think this is worth it, but just pointing out here that this is about the limit in my mind.
|
|
||
| # Handle external signals (parse and pass to individual components) | ||
| if "external_signals" in h_dict: | ||
| external_signals = h_dict["external_signals"] |
There was a problem hiding this comment.
Is this change necessary? In my mind, this obfuscates the code---it makes it less clear that the external_signals are coming from the passed-in h_dict. It's also meaning quite a few changes to the code, so unless it actually improves performance meaningfully, I'd rather not take it because it puts a seemingly unnecessary change in the version history for these lines
There was a problem hiding this comment.
Ok, ran some tests and almost all the benefit is from LMP key pre-allocation. Will revert the other changes
| for k, v in external_signals.items(): | ||
| if "forecast" in k: | ||
| measurements["forecast"][k] = h_dict["external_signals"][k] | ||
| measurements["forecast"][k] = v |
There was a problem hiding this comment.
In this case, I think the suggested form probably is a little more pythonic, so I'm ok with this change (possibly after reverting to h_dict["external_signals"].items())---but it's still a bit doubtful to me that there is a real performance enhancement
How much of the 2.5x speedup comes from each of these changes? (does Copilot respond to questions in this format? Seems not, unfortunately) |
|
Thanks for your comments @misi9170 , the changes should now be limited to those most worthwhile |
paulf81
force-pushed
the
feature/preallocate_measurements
branch
from
7e4e721 to
7910e5a
Compare
|
Merged develop back to make sure all in line with new ruff formatting rules---seems to be the case, so I'll go ahead and merge once checks pass. |
2 participants
Some of the lines in
get_measurementsunnecessarily slow the function down. This small PR gains speed up by:h_dict["external_signals"]intoexternal_signalsProfiling suggests this is worth a 2.5x speedup, going to run a full case to confirm